feat: add competency criteria models for CBE authoring layer by jesperhodge · Pull Request #800 · openedx/openedx-core

jesperhodge · 2026-09-01T20:23:40Z

Adds the authoring and definition half of the competency-based education (CBE) data model, per
ADR-0002
and ADR-0003.
Three models, one column on an existing model, two migrations, four configuration edits. No REST
endpoints, no UI, no evaluation logic.

Closes #641.

Warning

The deletion behavior here is work in progress and will change within this PR, pending
discussion with @mgwozdz. Three separate things are still open: the individual on_delete
values, the cascade-versus-protect split as a design question, and the archive-versus-delete
story as a whole. ADR-0002 Decision 7 was amended twice in three days (f9ec022, then
b5fae6b) while this branch was being written, and the second amendment reversed the reasoning
behind the first. Treat the on_delete table below as the current position, not a settled one.
Nothing else in this PR depends on how that discussion resolves.

Note

Implemented by an AI agent (Claude Code), with a human directing the work and reviewing the
decisions. Please review it as you would any other PR.

The three models

CompetencyCriteriaGroup is an internal AND/OR node of a criteria tree. A tree hangs off one
competency, which is a Tag in a competency-enabled taxonomy.
CompetencyCriterion is a leaf. It points at one ObjectTag, meaning one specific piece of
tagged content, and takes its pass rule either from a shared profile or from its own inline
override pair.
CompetencyRuleProfile is a reusable set of evaluation settings, scoped to at most one of an
organization, a course, or a taxonomy. One row is scoped to none of them: the system default,
seeded by migration, which every criterion falls back to when nothing more specific applies. In
this MVP it is the only profile that exists, so all three scope columns are always null.

Plus CompetencyTaxonomy.taxonomy_overrides_org, the boolean ADR-0002 Decision 1 asks for.

Decisions

scope_code is an ordinary column written in save(), null when the profile is archived, rather
than a database GeneratedField. At most one profile may exist per distinct scope, and a unique
constraint over the three nullable scope columns cannot enforce that, because SQL never treats two
NULLs as equal. The idiomatic fix, a conditional UniqueConstraint, compiles to a partial index
that MySQL silently skips (ADR-0002 Rejected Alternative 6). Deriving the value works, but deriving
it in the database broke twice: an archived profile kept occupying its scope's unique slot, so no
replacement could ever be created for that scope; and Django's collector nulls a nullable foreign
key before deleting the row it points at, which recomputed scope_code mid-delete and collided with
the seeded default row. The collector only does this where can_defer_constraint_checks is false.
MySQL has that flag false and SQLite has it true, so this failed only on MySQL. Writing the column
in save() fixes both: the collector's update no longer rewrites it, and archived rows carry
NULL, so any number of them share a scope while exactly one live row holds it, identically on
every backend. A CheckConstraint ties archived and scope_code together, so a
QuerySet.update() bypassing save() is refused by the database rather than silently breaking the
invariant. This deviates from #641, which asks for a generated, never-null column; ADR-0002
Decision 3 needs a matching amendment.

Validation and immutability each collapsed to one path. save() now calls full_clean() on
both models rather than repeating a hand-picked list of checks that could drift from clean(),
following the precedent CourseRun.save() sets. Scope immutability drops its cached copy of the
loaded scope and its from_db() override in favor of always reading the persisted scope, on
self._state.db so a non-default database alias is not silently skipped. The rule payload schema
moved out of the models module into rule_payloads.py and now returns the parsed GradeRule rather
than discarding it, so #642's evaluation code can consume typed fields without importing five
models. Both models derive their rule_type choices from the payload-spec registry, so a rule type
can never be offered to an author and then rejected on save.

Two on_delete values differ from #641, and this is the part under discussion.
CompetencyRuleProfile.course becomes CASCADE because b5fae6b says a profile is deleted along
with "a taxonomy or course" it is scoped to. CompetencyCriteriaGroup.course becomes CASCADE for
that amendment's own stated reason: a course is only hard-deleted once nothing beneath it needs
protecting, so a course-scoped criteria tree is safe to remove with it rather than blocking the
delete permanently. CompetencyRuleProfile.organization stays PROTECT, since the amendment names
only taxonomy and course, and an organization is not a competency-definition record. That asymmetry
is deliberate and is one of the things to settle.

Foreign key	Value
`CompetencyCriteriaGroup.tag`, `.parent`, `.course`	`CASCADE`
`CompetencyCriterion.group`, `.object_tag`	`CASCADE`
`CompetencyRuleProfile.competency_taxonomy`, `.course`	`CASCADE`
`CompetencyCriterion.rule_profile`	`PROTECT`
`CompetencyRuleProfile.organization`	`PROTECT`

Other deviations from #641

.importlinter ranks openedx_content above openedx_catalog rather than making them
independent siblings. The sibling form forbids imports both ways, including the direction
0007-pathway-catalog-content-split.rst requires, so it would have to be loosened to do an
already-decided thing.
RuleType declares only Grade. ADR-0002 also names View and MasteryLevel, but neither has a
payload shape, so declaring them offers an author a choice that always fails on save. Adding one
later means a spec class, a registry entry, and the matching member together.

Known gaps

Deleting a CompetencyTaxonomy whose taxonomy-scoped profile is assigned to a criterion raises
ProtectedError. Django's collector looks up referencing rows in the database rather than in the
set it has already decided to delete, so CompetencyCriterion.rule_profile's PROTECT fires even
for criteria being deleted in the same operation. This is unreachable in the MVP, where the only
profile is the system default. A test pins it and names the fix: a fifth reassignment event in
ADR-0002 Decision 4, in an application-layer function.
No test proves the scope-immutability query targets self._state.db. That needs a second database
alias, and configuring one breaks the whole test session on a pre-existing bug in
openedx_content/backcompat/collections/migrations/0004_collection_key.py, whose generate_keys
step queries without .using(schema_editor.connection.alias) and so always hits default.
Worth its own issue.
Out of scope: the application-layer archive-and-reassign function, the archive-versus-delete
branch from [Arch] Implementation approach for competency data delete/edit guardrails #655, the archived column on the group and leaf models ([BE] Add archived field to CompetencyCriteriaGroup and CompetencyCriterion #716), and Django admin
registration.

Testing

Every acceptance criterion in #641 has a test, written before the implementation. Tests are named
for the behavior they assert rather than the mechanism, and test_criteria_trees.py covers
whole-tree deletion, so a test proves the bad outcome is avoided rather than only that a cascade
fired. The deletion paths also run under MySQL's collector semantics while still on SQLite, by
setting can_defer_constraint_checks to false, which makes this class of bug visible in the fast
local suite instead of only in CI.

Verified against both backends, since a green SQLite run is not evidence for the scope_code
work: 887 passed on SQLite, 888 on a real MySQL 8. mypy, pylint, pycodestyle, pydocstyle and
isort are clean, lint-imports keeps both contracts, and makemigrations --check reports no drift
in openedx_learning. No # noqa, # pylint: disable, # type: ignore or TODO anywhere in the
diff.

make pii_check still fails at two pre-existing lint conflicts (openedx_content.Draft,
openedx_content.PublishableEntityVersion), both identical on main and in code this PR does not
touch. The models added here are annotated, including the three Historical* models
django-simple-history generates.

openedx-webhooks · 2026-09-01T20:23:46Z

Thanks for the pull request, @jesperhodge!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

jesperhodge · 2026-09-04T16:17:21Z

+    OR = "OR", _("Or")
+
+
+def validate_rule_payload(rule_type: str, payload: Any) -> None:


I don't like payload: Any, there should be a type for the dict.
And I want to validate against that type.

jesperhodge · 2026-09-04T16:33:27Z

+    .. no_pii:
+    """
+
+    # Set at from_db() time to the scope this row had when it was loaded from the database, so


Comment too hard to understand

jesperhodge · 2026-09-04T16:38:13Z

+    # from_db() is a classmethod, so it sets this through a local `instance` variable rather than
+    # `self`, which pylint's protected-access check can't tell apart from reaching into another
+    # object's internals.
+    loaded_scope: tuple[int | None, int | None, int | None] | None = None


What is loaded_scope and why is it a tuple of ints?

jesperhodge · 2026-09-04T16:41:31Z

+        Organization,
+        null=True,
+        blank=True,
+        on_delete=models.PROTECT,


organization & course on_delete should also be CASCADE, with Python logic elsewhere ensuring that no learners are linked to this (if they are linked, organization / course can still be deleted but rule profile stays.)

I think this is not quite right. CASCADE should result in archival, not deletion.
Question to flag for later: what should happen if someone actually wants to modify (or rather, delete and then replace) a rule profile? Assuming this gets archived, do the archived ones still need to be unique? In that case they are never modifiable and we need some hard delete or overwrite mechanism, I guess.

PROTECT is no good. The org delete shouldn't be blocked. Instead, use models.SET() to run code to archive the rule_profile, but only if no learners are connected.

jesperhodge · 2026-09-04T16:48:03Z

+        CourseRun,
+        null=True,
+        blank=True,
+        on_delete=models.PROTECT,


This also needs to be CASCADE but then it should stay if there are any actual learner's already connected to the mastery. Same as elsewhere

jesperhodge · 2026-09-04T16:55:57Z

+        """Capture the scope this row had when loaded, so clean()/save() can detect an edit to it."""
+        instance = super().from_db(db, field_names, values)
+        # field_names holds attnames (e.g. "organization_id"), not field names. Only capture when


I have no idea what this is supposed to mean. Clarify what the intent is here.

Why do we need to override from_db at all?

jesperhodge · 2026-09-04T17:00:59Z

+            )
+        return instance
+
+    def _check_scope_immutable(self) -> None:


This seems pretty complicated code. Can it be simplified following KISS principle?

jesperhodge · 2026-09-04T17:42:04Z

+        help_text=_("The profile this criterion uses by default. Null only when overrides are set instead."),
+    )
+    rule_type_override = models.CharField(max_length=32, choices=RuleType, null=True, blank=True)
+    rule_payload_override = models.JSONField(null=True, blank=True)


Use attrs, as in my other comment.

jesperhodge · 2026-09-04T17:43:02Z

+        null=True,
+        blank=True,
+        db_column="competency_rule_profile_id",
+        on_delete=models.PROTECT,


I think this is okay because rule profiles should be archived not deleted in general.

jesperhodge · 2026-09-04T17:46:03Z

+    )
+    uuid = immutable_uuid_field()
+
+    history = HistoricalRecords(excluded_fields=["scope_code"])


I wonder if the history and the archived flag somehow conflict with each other.

Also I wonder about this immutability idea anyway: if this should be immutable why a history?
Maybe immutability is not a clear concept in the issue. This will need further clarification from the architect.

jesperhodge · 2026-09-04T17:52:03Z

+# ==============================================================================================
+
+
+def test_group_parent_cascade(tag: Tag) -> None:


Test names are bad. They should all state the expected behavior. I don't care if that makes them long. E.g. "test_criteria_group_deletion" is bad, while "test_delete_criteria_group_cascades_to_child_groups" is good.

Here's some reading for proper naming of tests. https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices

jesperhodge · 2026-09-04T17:56:37Z

+
+def test_group_parent_cascade(tag: Tag) -> None:
+    """
+    Deleting a CompetencyCriteriaGroup cascades to any child group referencing it via `parent`:


At least some of the tests need to be a bit more integrative. In that: sure, we have tested that the cascade is there, but it's not clear why. There should be at least some tests that look at the actual bad outcome that we want to avoid: for example, do we suddenly have orphaned child groups that do not serve any purpose?

jesperhodge · 2026-09-04T17:57:30Z

+
+
+# ==============================================================================================
+# Transitive deletion tests required by #641's Deletions criteria: deleting an oel_tagging.Tag,


Too unclear. I have no patience to decipher what this comment means. Either it's clear at a glance or it's useless.

Implements the authoring and definition half of the CBE data model from ADR-0002: CompetencyCriteriaGroup (internal AND/OR nodes), CompetencyRuleProfile (reusable scoped evaluation defaults) and CompetencyCriterion (leaf nodes). Also adds the taxonomy_overrides_org column that PR openedx#712 left off CompetencyTaxonomy. CompetencyRuleProfile.scope_code is a generated, never-null column with a plain unique constraint. SQL never treats two NULLs as equal, so a unique constraint over the three nullable scope columns would accept two rows with the same scope, and the conditional UniqueConstraint that would normally fix that compiles to a partial index MySQL does not support. Both structural invariants are database check constraints rather than clean() checks, since DRF serializers, QuerySet.update() and bulk_create() never call full_clean(). Payload shape validation stays in clean(), per the issue. Every new foreign key is on_delete=PROTECT with a TODO(openedx#799) comment. That is a fail-closed placeholder, not a per-key decision; openedx#799 sets the real values once openedx#655 lands. openedx_catalog joins .importlinter's root_packages and the src_layering contract, since CompetencyCriteriaGroup.course is the first foreign key from openedx_learning into that app. django-simple-history moves into base.in: it was only ever a transitive dependency of edx-organizations, and setup.py builds install_requires from base.in. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

openedx#655 closed with an approved design and openedx#799 is now closed as superseded, so both halves of the nine repeated TODO comments were false: openedx#799 does not own the on_delete question, and no follow-up ticket will set "the real" per-foreign-key values. Replaces those nine identical comments with one explanation in the module docstring, which also records the open question openedx#655's design creates for CompetencyCriteriaGroup.tag and CompetencyCriterion.object_tag: that design keeps openedx_tagging ignorant of CBE and promises a plain hard delete for a tag no learner holds mastery against, which PROTECT turns into a ProtectedError whenever an author's criteria tree references the tag and nobody has been graded yet. The PROTECT values themselves are unchanged. They remain the fail-closed default until openedx#655's reviewers settle the question. Refs openedx#641, openedx#655 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

openedx#641 requires at least one test per foreign key asserting that deleting the referenced row matches what the field declares. All nine are PROTECT, so all nine assert ProtectedError, and each inspects ProtectedError.protected_objects rather than only the exception type: a single delete can trip several protected relationships, so a bare pytest.raises would not prove which foreign key did the protecting. Two cases needed isolating to avoid passing for the wrong reason. CatalogCourse.org is itself PROTECT, so the organization test uses an organization with no catalog course attached. Tag.taxonomy is CASCADE, so the competency_taxonomy test omits the tag and group fixtures. A tenth test pins the open openedx#655 question in executable form: deleting a CompetencyTaxonomy whose tag carries a criteria tree raises ProtectedError today, though that design promises the delete succeeds when no learner status exists. It is the test that has to change if the reviewers move CompetencyCriteriaGroup.tag to CASCADE, and says so. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

openedx#655 decided the on_delete question on 2026-09-02, so the four foreign keys between definition tables become CASCADE: CompetencyCriteriaGroup.parent, CompetencyCriteriaGroup.tag, CompetencyCriterion.group and CompetencyCriterion.object_tag. The other five stay PROTECT and are now final. Deleting a Tag nobody holds mastery against has to succeed, and openedx#655's design forbids openedx_tagging from knowing CBE exists, so the tagging-side path cannot clear the criteria tree first. CASCADE lets the delete take the tree with it. parent and group need it too, because Django's collector looks up referencing rows in the database rather than in the set it has already collected, so a parent and child reached in one batch would trip PROTECT and abort the walk partway down. This does not weaken ADR-0002 Decision 7. The four CASCADE links are what carries the collector down to the PROTECT that enforces it, on openedx#642's Student*Status foreign keys one and two levels below the tag, which Django reaches only by walking CASCADE edges. Migration 0002 is edited in place rather than gaining an AlterField, since it is unmerged. The delete tests are reworked accordingly and extended with the transitive cases: a tag delete cascading a whole tree, a group delete at depth taking its descendants and their criteria, and a taxonomy delete reaching through tag to group to criterion. The matching "raises ProtectedError when a learner status exists" halves need openedx#642's tables and belong to that slice, which a comment in the test file records. One cascade test also asserts django-simple-history writes a history_type='-' row per removed row, so the cascade is not silent for audit. Refs openedx#641, openedx#655 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…dx#641 Four changes, all from openedx#641's revision. CompetencyRuleProfile.competency_taxonomy becomes CASCADE, making the split five CASCADE and four PROTECT. The reason is a requirement rather than a mechanism: a rule profile must never be why a taxonomy delete fails. Once taxonomy-scoped profiles exist, deleting a taxonomy has to be blocked only when learner data is connected to it, and that check belongs in Python at the application layer, the way openedx#655 settled it for every other record type. PROTECT would push the decision into the database, which cannot tell the two cases apart. Nothing changes behaviorally in MVP, because the only profile is the system default and its three scope columns are all null. openedx_content and openedx_catalog become independent siblings in the src_layering contract rather than separate ranks. A layers contract is a strict total order, so ranking them asserted both that openedx_content may import openedx_catalog and that openedx_catalog may never import openedx_content. src/openedx_catalog/ARCHITECTURE.md records that direction as explicitly undecided, so the sibling form, which forbids imports both ways, asserts only what is settled. The Meta.db_table override is dropped, so the leaf table is Django's default openedx_learning_competencycriterion. ADR-0002 Decision 4's heading names a domain concept rather than instructing a rename, and no model anywhere in src/ overrides db_table. The competency_taxonomy delete test becomes a cascade test to match. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Replaces the hand-rolled key-walking in validate_rule_payload with one attrs class per rule type, using the modern `from attrs import define, field` style already used in openedx_tagging and openedx_content. attrs is already a declared dependency, so nothing changes in requirements. The spec class is now the definition of the shape: constructing it does the checking, and the expected key set is derived from it via attrs.fields() rather than repeated in a literal. Adding MasteryLevel later is one class plus one registry entry. The field validators stay hand-written rather than using attrs.validators.in_(), because that helper's default message dumps the whole Attribute repr into the error, which a course author would see in the Django admin. Key errors are raised before construction for the same reason: Python's own TypeError names the offending key but leaks "GradeRule.__init__()" along with it. validate_rule_payload is now also called from save() on both models. clean() is reached only via full_clean(), so objects.create() and instance.save() previously bypassed payload validation entirely; this closes both. QuerySet.update(), bulk_create() and DRF serializers remain uncovered, because none of them builds or saves a model instance, and both model docstrings say so rather than implying more. CourseRun.save() is the existing precedent in this repo for validating in save(). One consequence, split rather than papered over: a criterion with rule_type_override set and no payload now raises ValidationError from save() before the check constraint sees it, so that case moves out of test_criterion_profile_xor_override_constraint into its own test. The other three invalid states still reach the constraint and still raise IntegrityError. The seed data migration is unaffected: apps.get_model() returns a historical model that does not carry the custom save(). Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Add simple_history to INSTALLED_APPS in the test and dev settings. The CBE models declare HistoricalRecords(), and while the historical models are built under openedx_learning's own app label and so work without the entry, its absence breaks SimpleHistoryAdmin's history views, its template tag libraries and the populate_history/clean_old_history/clean_duplicate_history commands. The package ships no AppConfig and registers no system check, so nothing warns. Rank openedx_content above openedx_catalog in the src_layering contract rather than making them independent siblings. The sibling form forbids imports in both directions, including the one 0007-pathway-catalog-content-split.rst requires: "openedx_content knows about openedx_catalog, never the reverse." Ranking asserts only the settled half, that catalog never reaches up into content, and does not have to be loosened when pathway content lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

scope_code becomes an ordinary column written in save(), null when the profile is archived, keeping the plain UniqueConstraint and adding a CheckConstraint tying the two. This fixes three defects. An archived profile used to occupy its scope's unique slot forever, so no replacement could ever be created for that scope; SQL never treats two NULLs as equal, so archived rows now share a scope freely while exactly one live row holds it, identically on MySQL and SQLite. A database GeneratedField was also rewritten whenever Django's collector nulled a nullable scope foreign key before deleting, which it does on any backend where can_defer_constraint_checks is false, colliding with the seeded default row on MySQL while passing on SQLite. A plain column is not rewritten by that update. It also avoids Django never populating a GeneratedField in memory on MySQL. Two on_delete values change, per ADR-0002 Decision 7 as amended by b5fae6b. CompetencyRuleProfile.course becomes CASCADE, which that amendment requires when it says a profile is deleted with "a taxonomy or course" it is scoped to. CompetencyCriteriaGroup.course becomes CASCADE for the same stated reason: a course is only hard-deleted once nothing beneath it needs protecting, so a course-scoped criteria tree is safe to remove with it rather than blocking the delete. Both deviate from openedx#641, which lists them as PROTECT. Drop the loaded_scope cache and the from_db() override; _check_scope_immutable() now always reads the persisted scope, on self._state.db so a non-default alias is not silently skipped. save() calls full_clean() on both models instead of duplicating a hand-picked validation list that could drift from clean(). Move RuleType, the payload spec classes and the parser to rule_payloads.py, so the JSON schema is not trapped behind a module importing five models, and have it return the frozen GradeRule rather than discarding it. Both models derive their choices from the payload-spec registry, so a rule type can never be offered to an author and then rejected on save. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…dge cases Add a test per acceptance criterion, plus the cases the previous per-foreign-key shape could not reach: a taxonomy or course run deleted with a scoped rule profile, two taxonomies deleted together, an archived profile's scope being reused, and an ObjectTag delete leaving a childless group behind. Add test_criteria_trees.py for whole-tree deletion, so a test proves the bad outcome is avoided rather than only that a cascade fired: it builds a root/branch/grandchild tree with criteria at two levels and a mix of profile-assigned and override criteria, deletes in the middle, and asserts exactly which rows survive. Run the deletion paths under MySQL's collector semantics while still on SQLite, by setting can_defer_constraint_checks to False. That is what makes this class of bug visible in the fast local suite instead of only in the MySQL CI job. Rename every test so the name states the expected behavior rather than the mechanism, and move the fixtures duplicated across both files into conftest.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Decision 3 and Rejected Alternative 6 described scope_code as a database-generated, never-null column; the code has always shipped a plain, nullable-while-archived column instead. Amend both to match, and add the on_delete containment rationale and the taxonomy-delete known limitation to Decision 7, so the reasoning that was living only in inline comments and test docstrings has one authoritative home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…pecs GradeRule's docstring claimed construction itself was the validation, but the caller derived the expected key set from the class and checked it beforehand specifically to avoid attrs' TypeError leaking GradeRule.__init__ to whoever edits a payload, so construction only ever reached the three field validators. Replace the class, its validators, the introspection, and the unused parse_rule_payload wrapper with one plain validate_rule_payload function and a frozenset-plus-callable registry entry per rule type, keeping every message the existing tests assert on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… from RuleType Cuts the module docstring, the scope_code/UniqueConstraint/_check_scope_immutable comments, and the Meta comments on CompetencyCriteriaGroup and CompetencyCriterion down to what a reader needs, now that the argued-out rationale lives in ADR-0002 (previous commit). Extracts CompetencyRuleProfile's scope_code expression into _compute_scope_code() so save() reads as two statements instead of one nested conditional. Drops _RULE_TYPE_CHOICES, a manual re-derivation of RuleType.choices that forced this module to import a private registry from rule_payloads.py just to prove an invariant a test already covers; both fields now declare choices=RuleType directly, which is a no-op migration. RuleType and validate_rule_payload come out of this module's __all__ since it does not define them, and models/__init__.py now imports RuleType from rule_payloads.py directly so it is re-exported from one place instead of two. Syncs migration 0002's scope_code help_text with the model, since this branch's migration 0002 has never been applied outside local development. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… 0003 consistently The simple_history INSTALLED_APPS comment, duplicated verbatim in projects/dev.py and test_settings.py, justified the entry by SimpleHistoryAdmin and management commands this change doesn't use; replace both with one line stating why the app is required at all. Switch migration 0003's string quoting from single to double to match the hand-written code around it; the UUID, the scope_code literal, and both RunPython functions are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Merges each model's pair of schema-mirror tests (columns-match / no-columns-beyond) into one test asserting the exact field set, the nullable subset, and every db_column/remote_field.model assertion the pair had; drops assertions that only restated the line above them in the model. Fixes the two test_criteria_models.py and test_criteria_deletion.py references to DECISION-on-delete.md, a file that does not exist in this repository, now that ADR-0002 records the scope_code and on_delete reasoning they pointed at. Cuts the test-module banner comments down to what a reader needs, pointing at the ADR for the argument instead of repeating it, and removes change-narration ("used to be", "before this", "do not simplify") and unresolvable AC-number lists in favor of stating the fact that makes each test necessary. Renames several tests to state the behavior under test rather than its subject, and parametrizes the AND/OR/null logic_operator test so each value reports independently instead of hiding behind the first failure in a loop. Deletes two tests, called out here for sign-off: test_uuid_is_a_stable_unique_ non_editable_external_identifier, which asserted immutable_uuid_field()'s own contract rather than anything about these three models, and test_scope_code_ unique_constraint_is_unconditional, whose MySQL rationale now lives in the ADR and whose behavior is already covered by test_two_live_profiles_cannot_share_the_same_ scope and test_archiving_a_profile_frees_its_scope_for_a_replacement. Also drops four "GradeRule"/"__init__" absence assertions from two message-quality tests, now meaningless since the attrs-based GradeRule class no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

One docstring cited "the fix described in this section's header" after that header stopped narrating a bug, and another cited openedx#641's AC25, which a reader cannot resolve without the ticket open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…-competency-criteria-models

jesperhodge · 2026-09-08T19:08:36Z

   3. ``course_id``: The ``course_id`` of the course that this competency rule profile is scoped to. Null if it is not scoped to a specific course.
   4. ``competency_taxonomy_id``: The ``CompetencyTaxonomy.taxonomy_ptr_id`` of the competency taxonomy that this competency rule profile is scoped to. Null if it is not scoped to a specific taxonomy.
-   5. ``scope_code``: A database-generated column that is always in the fixed, trivially-parseable format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, ``"org:,course:12,taxonomy:"``, ``"org:,course:,taxonomy:7"``, or ``"org:,course:,taxonomy:"`` for the system default. ``scope_code`` is therefore never null, including for the system default row. This exists because SQL never treats two ``NULL`` values as equal for uniqueness purposes, so a plain unique constraint across the three nullable scope columns would not stop two rows from sharing the same scope (for example two rows both with ``organization_id=5`` and the other two columns null). Collapsing the scope into one generated, always-non-null column sidesteps that, and does so identically on every database backend this project supports, including MySQL, which does not support the conditional/partial unique indexes that would otherwise be the usual fix. ``scope_code`` embeds internal ID references and exists solely to enforce uniqueness; it is not intended to be exported or exposed outside this system.
+   5. ``scope_code``: A plain column, recomputed by the model's ``save()`` and never set directly, in the fixed, trivially-parseable format ``"org:X,course:Y,taxonomy:Z"``, with each segment left blank when the corresponding scope column is null: for example ``"org:5,course:,taxonomy:"``, or ``"org:,course:,taxonomy:"`` for the system default row. It is null while a profile is archived, and non-null while it is live. Collapsing the scope into one column exists because SQL never treats two ``NULL`` values as equal for uniqueness purposes, so a plain unique constraint across the three nullable scope columns would not stop two rows from sharing a scope. Nulling it while archived is what frees an archived profile's scope for a replacement, without needing the conditional unique index MySQL does not support. It is a plain column rather than a ``GeneratedField`` because Django's delete collector nulls a nullable cascading foreign key before issuing the DELETE on backends that cannot defer constraint checks, and a generated column would recompute from that nulled value and collide with whichever row already holds the resulting blank scope. A plain column is untouched by that nulling.


But isn't archiving an operation that is intended to possibly restored later? If we remove the scope, don't we loose that data? Maybe we should do `"org:X,course:Y,taxonomy:Z" with possible blank segments, but for archived items, we do something like "org:X,course:Y,taxonomy:Z,archive-version:1" to keep it restorable and unique

jesperhodge · 2026-09-08T19:10:25Z

   - Once a related row exists in ``StudentCompetencyCriteriaStatus``, deletion of the associated competency definition row still succeeds, but as an archive (soft delete) instead of a hard delete: the row is hidden from authoring and new associations but remains queryable, so existing learner status rows stay resolvable. This archive-vs-hard-delete rule applies to ``oel_tagging_tag``, ``oel_tagging_taxonomy``, ``CompetencyTaxonomy``, ``oel_tagging_objecttag``, ``CompetencyCriteriaGroup``, and ``CompetencyCriteria``; see :ref:`openedx-learning-adr-0003` Decision 3 for ``oel_tagging_objecttag``'s own archive rule and traceability exception.
   - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet.
   - Direct deletion of a ``CompetencyRuleProfile`` is never a hard delete; retirement is always archive-only, via a normal update to its ``archived`` column (Decision 3). However, if a taxonomy or course that is associated with a taxonomy- or course-scoped profile is deleted, then this profile will be deleted along with it.
+   - ``on_delete`` on the criteria tables expresses containment, not protection: a row whose referent is gone is meaningless, so ``CompetencyCriteriaGroup.parent``, ``.tag`` and ``.course``, ``CompetencyCriterion.group`` and ``.object_tag``, and ``CompetencyRuleProfile.course`` and ``.competency_taxonomy`` all cascade. ``CompetencyCriterion.rule_profile`` stays ``PROTECT``, which is what makes "a profile is never hard-deleted by a direct delete" hold at the ORM layer. ``CompetencyRuleProfile.organization`` stays ``PROTECT`` because an ``Organization`` is not a competency definition record and ``edx-organizations`` deactivates organizations rather than deleting them. The tree links additionally have to cascade for a mechanical reason: Django's collector looks up referencing rows in the database rather than in the set it has already decided to delete, so a parent and child reached in the same batch would still trip ``PROTECT`` and abort the walk partway down. Those cascading edges are what carries a delete down to the ``PROTECT`` on the learner status tables, which is where this decision is actually enforced.


Unreadable, unclear

jesperhodge · 2026-09-08T19:11:02Z

   - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet.
   - Direct deletion of a ``CompetencyRuleProfile`` is never a hard delete; retirement is always archive-only, via a normal update to its ``archived`` column (Decision 3). However, if a taxonomy or course that is associated with a taxonomy- or course-scoped profile is deleted, then this profile will be deleted along with it.
+   - ``on_delete`` on the criteria tables expresses containment, not protection: a row whose referent is gone is meaningless, so ``CompetencyCriteriaGroup.parent``, ``.tag`` and ``.course``, ``CompetencyCriterion.group`` and ``.object_tag``, and ``CompetencyRuleProfile.course`` and ``.competency_taxonomy`` all cascade. ``CompetencyCriterion.rule_profile`` stays ``PROTECT``, which is what makes "a profile is never hard-deleted by a direct delete" hold at the ORM layer. ``CompetencyRuleProfile.organization`` stays ``PROTECT`` because an ``Organization`` is not a competency definition record and ``edx-organizations`` deactivates organizations rather than deleting them. The tree links additionally have to cascade for a mechanical reason: Django's collector looks up referencing rows in the database rather than in the set it has already decided to delete, so a parent and child reached in the same batch would still trip ``PROTECT`` and abort the walk partway down. Those cascading edges are what carries a delete down to the ``PROTECT`` on the learner status tables, which is where this decision is actually enforced.
+   - Known limitation: deleting a ``CompetencyTaxonomy`` whose taxonomy-scoped profile is assigned to a ``CompetencyCriterion`` raises ``ProtectedError`` naming that criterion, even though the criterion would also be cascade-deleted in the same operation through the tag chain, for the same collector reason above. This is unreachable until scoped profiles can be authored. The fix at that point is a fifth reassignment event on Decision 4: when a profile's scope owner is being deleted, reassign every criterion off that profile before the cascade proceeds.


Needs review

jesperhodge · 2026-09-08T19:17:57Z

+    Editing a profile may change ``rule_type``/``rule_payload`` only: the scope fields
+    (``organization``, ``course``, ``competency_taxonomy``) are immutable after creation, so that
+    criteria already resolved to this profile's scope are never silently re-governed. ``clean()``
+    enforces this by comparing the current scope columns against what is actually persisted for
+    this row, so the check holds regardless of whether this instance was loaded with a partial
+    ``.only()``/``.defer()`` that skipped some scope columns. It does not cover a bulk
+    ``QuerySet.update()``, since that path never loads or constructs a model instance at all.


Suggested change

Editing a profile may change ``rule_type``/``rule_payload`` only: the scope fields

(``organization``, ``course``, ``competency_taxonomy``) are immutable after creation, so that

criteria already resolved to this profile's scope are never silently re-governed. ``clean()``

enforces this by comparing the current scope columns against what is actually persisted for

this row, so the check holds regardless of whether this instance was loaded with a partial

``.only()``/``.defer()`` that skipped some scope columns. It does not cover a bulk

``QuerySet.update()``, since that path never loads or constructs a model instance at all.

Unnecessary explanation

jesperhodge · 2026-09-08T19:18:16Z

+    ``rule_payload``'s shape (see :func:`~openedx_learning.applets.cbe.rule_payloads.validate_rule_payload`)
+    is likewise validated from ``clean()``, reached from both ``objects.create()`` and a plain
+    ``instance.save()`` via ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a
+    DRF serializer that writes straight to the database are NOT covered: none of them build or save
+    a model instance, so ``clean()`` never runs.


Suggested change

``rule_payload``'s shape (see :func:`~openedx_learning.applets.cbe.rule_payloads.validate_rule_payload`)

is likewise validated from ``clean()``, reached from both ``objects.create()`` and a plain

``instance.save()`` via ``full_clean()``. A bulk ``QuerySet.update()``, ``bulk_create()``, and a

DRF serializer that writes straight to the database are NOT covered: none of them build or save

a model instance, so ``clean()`` never runs.

Unnecessary explanation

jesperhodge · 2026-09-08T19:22:28Z

+        CourseRun,
+        null=True,
+        blank=True,
+        on_delete=models.CASCADE,


Use models.SET() and archive. only if there are no learners connected

jesperhodge · 2026-09-08T19:22:43Z

+        CompetencyTaxonomy,
+        null=True,
+        blank=True,
+        on_delete=models.CASCADE,


jesperhodge · 2026-09-08T19:43:05Z

+            # A new, unsaved instance: there's no persisted scope yet to compare against.
+            return
+        # Queried rather than compared against a value cached at load time, so a deferred load or
+        # a refresh_from_db() cannot bypass the check. `using` keeps a non-default-database


What are we talking about specifically with non-default-database instance? Why would there be such a thing?

jesperhodge · 2026-09-08T19:43:26Z

+        # a refresh_from_db() cannot bypass the check. `using` keeps a non-default-database
+        # instance from being compared against the wrong alias.
+        persisted_scope = (
+            CompetencyRuleProfile.objects.using(self._state.db)


Suggested change

CompetencyRuleProfile.objects.using(self._state.db)

CompetencyRuleProfile.objects

jesperhodge · 2026-09-08T19:53:10Z

+        persisted_scope = (
+            CompetencyRuleProfile.objects.using(self._state.db)
+            .filter(pk=self.pk)
+            .values_list("organization_id", "course_id", "competency_taxonomy_id")


Better take all 3 values and compare them against all 3 current values

jesperhodge · 2026-09-08T20:01:53Z

+        scope_ids = (self.organization_id, self.course_id, self.competency_taxonomy_id)
+        return "org:{},course:{},taxonomy:{}".format(*("" if pk is None else pk for pk in scope_ids))


Suggested change

scope_ids = (self.organization_id, self.course_id, self.competency_taxonomy_id)

return "org:{},course:{},taxonomy:{}".format(*("" if pk is None else pk for pk in scope_ids))

return f"org:{self.organization_id},course:{self.course_id},taxonomy:{self.competency_taxonomy_id}"

jesperhodge · 2026-09-08T20:03:06Z

+        # Ensure that we run the validations/defaults defined in clean().
+        # But don't validate_unique(); it just runs extra queries and the database enforces it anyways.


Suggested change

# Ensure that we run the validations/defaults defined in clean().

# But don't validate_unique(); it just runs extra queries and the database enforces it anyways.

# validate_unique() is already enforced by the database.

jesperhodge · 2026-09-08T20:03:58Z

+        return "org:{},course:{},taxonomy:{}".format(*("" if pk is None else pk for pk in scope_ids))
+
+    def save(self, *args, **kwargs):
+        """Recompute scope_code, then persist this profile after full_clean() re-validates it."""


Suggested change

"""Recompute scope_code, then persist this profile after full_clean() re-validates it."""

"""On save: recompute and validate scope_code"""

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 1, 2026

openedx-webhooks added this to Contributions Sep 1, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Sep 1, 2026

jesperhodge mentioned this pull request Sep 2, 2026

feat: add mastery status lookup and the three learner status models #802

Draft

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Sep 2, 2026

jesperhodge commented Sep 4, 2026

View reviewed changes

jesperhodge and others added 9 commits September 5, 2026 08:52

jesperhodge force-pushed the jesperhodge/feat--641-competency-criteria-models branch from a6a8e76 to 5380a64 Compare September 5, 2026 15:14

jesperhodge and others added 6 commits September 8, 2026 09:49

Merge remote-tracking branch 'origin/main' into jesperhodge/feat--641…

13a98de

…-competency-criteria-models

jesperhodge commented Sep 8, 2026

View reviewed changes

		OR = "OR", _("Or")


		def validate_rule_payload(rule_type: str, payload: Any) -> None:

		# ==============================================================================================


		def test_group_parent_cascade(tag: Tag) -> None:



		# ==============================================================================================
		# Transitive deletion tests required by #641's Deletions criteria: deleting an oel_tagging.Tag,

	CompetencyRuleProfile.objects.using(self._state.db)
	CompetencyRuleProfile.objects

		scope_ids = (self.organization_id, self.course_id, self.competency_taxonomy_id)
		return "org:{},course:{},taxonomy:{}".format(*("" if pk is None else pk for pk in scope_ids))

	scope_ids = (self.organization_id, self.course_id, self.competency_taxonomy_id)
	return "org:{},course:{},taxonomy:{}".format(*("" if pk is None else pk for pk in scope_ids))
	return f"org:{self.organization_id},course:{self.course_id},taxonomy:{self.competency_taxonomy_id}"

		# Ensure that we run the validations/defaults defined in clean().
		# But don't validate_unique(); it just runs extra queries and the database enforces it anyways.

	# Ensure that we run the validations/defaults defined in clean().
	# But don't validate_unique(); it just runs extra queries and the database enforces it anyways.
	# validate_unique() is already enforced by the database.

	"""Recompute scope_code, then persist this profile after full_clean() re-validates it."""
	"""On save: recompute and validate scope_code"""

Uh oh!

Conversation

jesperhodge commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The three models

Decisions

Other deviations from #641

Known gaps

Testing

Uh oh!

openedx-webhooks commented Sep 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

jesperhodge commented Sep 1, 2026 •

edited

Loading